-
Notifications
You must be signed in to change notification settings - Fork 38.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Made tracing of calls and container lifecycle steps in FakeDockerClient optional #39826
Made tracing of calls and container lifecycle steps in FakeDockerClient optional #39826
Conversation
@yujuhong There are also some unused functions like AssertCreated, AssertStarted, AssertStopped, etc. Should we just let them stay there as available features? |
} | ||
|
||
func NewFakeDockerClientWithClock(c clock.Clock) *FakeDockerClient { | ||
return newClientWithVersionAndClock(fakeDockerVersion, minimumDockerAPIVersion, c) | ||
func (f *FakeDockerClient) WithClock(c clock.Clock) *FakeDockerClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to lock here and the following.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
} | ||
|
||
func NewFakeDockerClientWithVersion(version, apiVersion string) *FakeDockerClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some other places using this function. You also need to update them.
For example
fakeDockerClient := NewFakeDockerClientWithVersion("1.2.3", "1.2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Sorry for the blooper.
return f | ||
} | ||
|
||
func (f *FakeDockerClient) AppendCalledWith(callDetail calledDetail) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AppendCalled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
} | ||
|
||
func (f *FakeDockerClient) AppendPulledWith(pull string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AppendPulled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
} | ||
|
||
func (f *FakeDockerClient) AppendContainerTraceWithEntry(traceCategory string, containerName string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AppendContainerTrace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
func (f *FakeDockerClient) AppendContainerTraceWithEntry(traceCategory string, containerName string) { | ||
if f.EnableTrace == false { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: !f.EnableTrace
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
func (f *FakeDockerClient) AppendPulledWith(pull string) { | ||
if f.EnableTrace == true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if f.EnableTrace {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
df90817
to
194cc24
Compare
Jenkins Bazel Build failed for commit 194cc24234aed82776618264888123fa47ea4b37. Full PR test history. The magic incantation to run this job again is Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Jenkins unit/integration failed for commit 194cc24234aed82776618264888123fa47ea4b37. Full PR test history. The magic incantation to run this job again is Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@Random-Liu There is a very weird reason why the unit tests are failing. So in this line in container_gc_test.go file, we do a new(FakeDockerClient) which initializes the struct with all fields set to their zero value. So this disables tracing and hence these tests fail later on, when they assert non-empty expected traces to be same as these actual empty traces. Should we change the field in FakeDockerClient to DisableTrace or just change the line from |
@shyamjvs Just change this line should be fine. I don't think the original code is right. We should always use initializer to create the object. |
194cc24
to
e6f8a4b
Compare
@Random-Liu Can you PTAL now? Fixed the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one nit
return f | ||
} | ||
|
||
func (f *FakeDockerClient) AppendCalled(callDetail calledDetail) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use private function, because this function should only be used internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks for pointing.
} | ||
} | ||
|
||
func (f *FakeDockerClient) AppendPulled(pull string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
} | ||
|
||
func (f *FakeDockerClient) AppendContainerTrace(traceCategory string, containerName string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
e6f8a4b
to
814c850
Compare
Adding lgtm label based on @Random-Liu 's comment. |
@k8s-bot test this please. |
Automatic merge from submit-queue |
Fixes #39717
Slightly refactored the FakeDockerClient code and made tracing optional (but enabled by default).
@yujuhong @Random-Liu